Skip to content

Add Web IDL base types + Encoding Standard from their specs - #405

Merged
Mohamed Hegazy (mhegazy) merged 15 commits into
microsoft:masterfrom
saschanaz:widl
Apr 5, 2018
Merged

Add Web IDL base types + Encoding Standard from their specs#405
Mohamed Hegazy (mhegazy) merged 15 commits into
microsoft:masterfrom
saschanaz:widl

Conversation

@saschanaz

@saschanaz Kagami Sascha Rosylight (saschanaz) commented Apr 4, 2018

Copy link
Copy Markdown
Contributor

The new json type file tends to flatten typedefs, is it a requirement?

@@ -0,0 +1,23 @@
dictionary TextDecoderOptions {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is the idl file comming from?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind. found the link :)

Comment thread src/fetcher.ts Outdated
}

async function fetchIDLs() {
const file = fs.readFileSync(`${__dirname}/../inputfiles/idlSources.json`, { encoding: "utf-8" });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i would use path.combine here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or consider using require to get it and avoid JSON.parse

Comment thread src/fetcher.ts
async function fetchIDL(source: IDLSource) {
const response = await fetch(source.url);
const dom = new JSDOM(await response.text());
const elements = Array.from(dom.window.document.querySelectorAll("pre.idl:not(.extract),code.idl-code"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally random, and not related to this change; it would be super cool if in the future we can extract the description from the widl file and put it as a comment on the declaration..

Comment thread src/widlprocess.ts Outdated
throw new Error("Unsupported IDL type structure");
}

function createEmptyBrowserWebidl(): Browser.WebIdl {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have this function already in index.ts, i would move it helpers and reuse it.

Comment thread src/widlprocess.ts Outdated
export function convert(text: string) {
const rootTypes = webidl2.parse(text);
const partialInterfaces: Browser.Interface[] = [];
const browser = createEmptyBrowserWebidl();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, browser might not be the best name, consider changing it to webidl

@mhegazy Mohamed Hegazy (mhegazy) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, and the build seems to be failing on a few errors as well.

Comment thread src/widlprocess.ts Outdated
return {
name: operation.name!,
signature: [{
...convertIdlType(operation.idlType!),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we throe if idlType is not defined?

Comment thread src/test.ts Outdated
function compileGeneratedFile(file: string) {
try {
child_process.execSync(`node ${tscPath} --strict --lib es5 --noEmit ${path.join(outputFolder, file)}`);
child_process.execSync(`node ${tscPath} --strict --lib es5 --skipLibCheck --noEmit ${path.join(outputFolder, file)}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? we want to make sure we are generating valid declaration file.. that is the whole point of the test.

@saschanaz Kagami Sascha Rosylight (saschanaz) Apr 5, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks @types/jsdom which depends on DOM types. I think we are still testing the input library file, this just turns off checks for JavaScript types.

Edit: @types/jsdom only breaks when checking worker types, as it lacks DOM.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then Add ‘—types’ to exclude all Types (@types) packages.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, but how can I pass empty string to --types? --types [], --types "", or --types "''" don't work.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just --types..

--strict --lib es5 --types --noEmit

@mhegazy
Mohamed Hegazy (mhegazy) merged commit 65fceb8 into microsoft:master Apr 5, 2018
@mhegazy

Copy link
Copy Markdown
Contributor

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants